[cob_light] added new modes#411
[cob_light] added new modes#411JeroenKempen wants to merge 1 commit into4am-robotics:noetic-develfrom
Conversation
* added mode for a turnsignal to the left * added mode for a turnsingal to the right * optimized datatypes in different places
|
thanks for the contribution @benmaidel @LoyVanBeek |
LoyVanBeek
left a comment
LoyVanBeek
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
The PR adds a few handy modes but also a lot of reformatting and whitespace changes. The latter is not desirable, as this brings it out of line with other code in this overall project.
So bits have also been removed and commented out without a clear reason (that I can see). The whole PR is one commit which makes it hard to go through the reasoning in the code.
Splitting the PR is multiple commits that each have their own piece of work (add mode X, optimize datastructures, handle whitespace etc) would make things easier to review.
|
|
||
| std::string getName() | ||
| { | ||
| return std::string("SignalRIght"); |
| { | ||
| cols.push_back(_color); | ||
| } | ||
| for (uint16_t i = (_num_leds / 2) - leds_margin; i < _num_leds; ++i) |
There was a problem hiding this comment.
I think this is the only difference between the left and right modes, correct?
Is there really no way to have these both derive from a common class (eg. DirectionIndicatorMode or something better) and have a direction variable set to either left/right that makes the quintessential difference between these modes?
| ros::init(argc, argv, "cob_light"); | ||
| // signal(SIGINT, sigIntHandler); | ||
|
|
||
| // Override XMLRPC shutdown | ||
| ros::XMLRPCManager::instance()->unbind("shutdown"); | ||
| ros::XMLRPCManager::instance()->bind("shutdown", shutdownCallback); | ||
| // ros::XMLRPCManager::instance()->unbind("shutdown"); | ||
| // ros::XMLRPCManager::instance()->bind("shutdown", shutdownCallback); | ||
|
|
||
| // create LightControl instance |
There was a problem hiding this comment.
Why has this been removed?
| // color_tmp.r *= color.a; | ||
| // color_tmp.g *= color.a; | ||
| // color_tmp.b *= color.a; | ||
|
|
||
| color_tmp.r = fabs(color_tmp.r * 255); | ||
| color_tmp.g = fabs(color_tmp.g * 255); | ||
| color_tmp.b = fabs(color_tmp.b * 255); | ||
| // color_tmp.r = fabs(color_tmp.r * 255); | ||
| // color_tmp.g = fabs(color_tmp.g * 255); | ||
| // color_tmp.b = fabs(color_tmp.b * 255); |
There was a problem hiding this comment.
Why has this been removed?
|
|
||
| void StageProfi::setColorMulti(std::vector<color::rgba> &colors) | ||
| { | ||
| ROS_WARN(__PRETTY_FUNCTION__); |
There was a problem hiding this comment.
Should be ROS_DEBUG at best I guess, if still needed at all
fmessmer
left a comment
fmessmer
left a comment
There was a problem hiding this comment.
marking as "Changes Requested"
see https://github.com/ipa320/cob_driver/pull/411#pullrequestreview-364693335
3 participants
For the turnsignal to use simple state the color in witch the turn signal should be blinking and the frequency.